Allow backend to send commands to execute preamble scripts. #1122

klochek · 2024-02-07T21:01:38Z

Introduce a new function on __RECORD_REPLAY__ called registerGlobal, which receives one argument, the name of a global, that can be accessed by any descendant context (read: iframe, with appropriate sandboxing). registerGlobal stores the string in a new top-level global, __RECORD_REPLAY_GLOBALS__. Example usage:

  // ... js code here ...

  Target_evaluatePrivileged.chromiumRunPointEvaluationArg = ...;

  // Ensure Target_evaluatePrivileged is available across all descendant contexts.
  __RECORD_REPLAY_GLOBALS__.registerGlobal("Target_evaluatePrivileged");

Introduce a new command, Target.loadPreamble, which is given a string as an argument. The string is a script that will be immediately executed in the root (global) context by the browser. After execution, a second script is run which propagates all Replay globals declared in __RECORD_REPLAY_GLOBALS__ to all descendent child contexts, so that they can be accessed as if they were in the context's top-level scope.

Any time a new context is created, we re-execute the propagation code, which ensures the registered globals are available in that context.

Domiii · 2024-02-08T13:32:22Z

third_party/blink/renderer/bindings/core/v8/record_replay_interface.cc

@@ -823,6 +824,10 @@ const char* gOnNewWindowScript = R""""(
  //      __RECORD_REPLAY__?
  window.__RECORD_REPLAY__ = window.top.__RECORD_REPLAY__;
  window.__RECORD_REPLAY_ARGUMENTS__ = window.top.__RECORD_REPLAY_ARGUMENTS__;
+
+  for (g of window.top.__RECORD_REPLAY_GLOBALS__) {


We also need to pass down __RECORD_REPLAY_GLOBALS__ itself.

In fact, I'd vote to only pass down __RECORD_REPLAY_GLOBALS__, and just force any user of this API to go through that, rather than adding random globals into a global context that is shared with arbitrary user JS.

→ linter failure.

Domiii · 2024-02-08T13:43:24Z

I want to see the API used and working before hitting Approve (if at all possible)

Domiii · 2024-02-08T14:20:18Z

third_party/blink/renderer/bindings/core/v8/record_replay_interface.cc

@@ -2502,6 +2547,10 @@ static void InitializeRecordReplayApiObjects(v8::Isolate* isolate, LocalFrame* l
  DefineProperty(isolate, context->Global(), "__RECORD_REPLAY_ARGUMENTS__",
                 args);

+  v8::Local<v8::Object> globals = v8::Object::New(isolate);
+  DefineProperty(isolate, context->Global(), "__RECORD_REPLAY_GLOBALS__",


This will reset the globals on every root-level navigation. We probably want to keep them around or re-run the preamble.

yeah, I was also confused about this. do we need to store the scripts somewhere so we can rerun them?

Domi and I have been chatting about our context handling, which is a tad naiive to say the least; we aren't going to worry about it in this PR, but rather address it wholly in another.

cweld510 · 2024-02-08T22:18:22Z

I want to see the API used and working before hitting Approve (if at all possible)

I can try to integrate with it.

cweld510

i'll attempt to try out these changes locally.

side question, not blocking: do we assume that all of this code is running single-threaded because it is only called into from v8, we only ever have one v8 instance, and the v8 instance is single-threaded?

cweld510 · 2024-02-09T13:52:02Z

replay-assets/replay_command_handlers.js

@@ -3043,6 +3050,10 @@ function replayEval(fn) {
  }
 }

+function registerGlobal(name, val) {


This is intended to be called by the preamble script, right?

Yup--the PR description shows a hypothetical usage.

cweld510 · 2024-02-09T14:00:08Z

third_party/blink/renderer/bindings/core/v8/record_replay_interface.cc

@@ -2350,6 +2355,46 @@ static void fromJsEndReplayCode(
  recordreplay::ExitReplayCode();
 }

+// Connects replay globals that must be accessible from all contexts; MUST be called
+// with AutoMarkReplayCode.
+static void ConnectPreambleGlobals(LocalFrame* frame, v8::Isolate *isolate) {


can you pass in the AutoMarkReplayCode object here to ensure that it is held?

Ehh, I waffled on this, due to recursion (it just looks distasteful/re-entrant to me); once we fix our context handling, I think we'll end up with a set of Conexts we can just safely iterate over, rather than this traversal.

cweld510 · 2024-02-09T14:24:34Z

third_party/blink/renderer/bindings/core/v8/record_replay_interface.cc

@@ -2350,6 +2355,46 @@ static void fromJsEndReplayCode(
  recordreplay::ExitReplayCode();
 }

+// Connects replay globals that must be accessible from all contexts; MUST be called
+// with AutoMarkReplayCode.
+static void ConnectPreambleGlobals(LocalFrame* frame, v8::Isolate *isolate) {


nit: maybe name should be ConnectPreambleGlobalsToCurrentContexts or something like that so it's clear what we are connecting the globals to?

cweld510 · 2024-02-09T14:28:20Z

third_party/blink/renderer/bindings/core/v8/record_replay_interface.cc

@@ -2502,6 +2547,10 @@ static void InitializeRecordReplayApiObjects(v8::Isolate* isolate, LocalFrame* l
  DefineProperty(isolate, context->Global(), "__RECORD_REPLAY_ARGUMENTS__",
                 args);

+  v8::Local<v8::Object> globals = v8::Object::New(isolate);
+  DefineProperty(isolate, context->Global(), "__RECORD_REPLAY_GLOBALS__",


yeah, I was also confused about this. do we need to store the scripts somewhere so we can rerun them?

klochek · 2024-02-09T15:52:04Z

i'll attempt to try out these changes locally.

side question, not blocking: do we assume that all of this code is running single-threaded because it is only called into from v8, we only ever have one v8 instance, and the v8 instance is single-threaded?

Yes, this is by V8's design.

replay-io · 2024-02-09T16:06:07Z

Status	In Progress ↗︎
Commit	`8805488`
Results	❌ 3 Failed breakpoints-07: rewind and seek using command bar and console messages (Replay 1, Replay 2, Replay 3) breakpoints-08: should be temporarily disabled (Replay 1, Replay 2, Replay 3) playwright-01: Basic Test Suites panel functionality (Replay 1, Replay 2, Replay 3) ⚠️ 2 Flaky authenticated/passport-03: Swiss army knife (Replay 1, Replay 2) breakpoints-05: Test interaction of breakpoints with debugger statements (Replay 1, Replay 2) ✅ 55 Passed authenticated/comments-01: Test add, edit, and delete comment functionality authenticated/comments-02: Test shared comments and replies (Replay 1, Replay 2) authenticated/logpoints-01: Shared logpoints functionality (Replay 1, Replay 2) authenticated/passport-01: Time travel authenticated/passport-02: Infrared inspection authenticated/passport-04: Multiplayer breakpoints-01: Test basic breakpoint functionality breakpoints-02: Test unhandled divergence while evaluating at a breakpoint breakpoints-03: Test stepping forward through breakpoints when rewound before the first one breakpoints-04: catch, finally, generators, and async/await console_async: support console evaluations in async frames console_dock: Should show the correct docking behavior for recordings with video console_eval: support console evaluations console_warp-01: should support warping to console messages console-expressions-01: should cache input eager eval and terminal expressions per instance cypress-04: Test Step buttons and menu item cypress-05: Test DOM node preview on user action step hover deleted-recording: Show error message for deleted recording elements-search: Element panel should support basic and advanced search modes fe-1875 :: verify that steps go to the right point in time file-search-01: should search files focus_mode-01: should filter messages as regions based on the active focus mode highlighter: element highlighter works everywhere inspector-elements-01: Basic DOM tree node display inspector-elements-02_node-picker: element picker and iframe behavior inspector-elements-05_search: element picker and iframe behavior logpoints-01: log-points appear in the correct order and allow time warping logpoints-02: conditional log-points logpoints-03: should display event properties in the console logpoints-05: should auto-complete based on log point location logpoints-06: should be temporarily disabled logpoints-07: should use the correct scope in auto-complete logpoints-08: should support jumping directly to a hit point via the capsule input logpoints-09: should support pending edits logpoints-10: too-many-points-to-find UX logpoints-11: too-many-points-to-run-analysis UX object_preview-02: should allow objects in scope to be inspected object_preview-03: Test previews when switching between frames and stepping object_preview-05: Should support logging objects as values object_preview-06: HTML elements playwright-02: Test Step timeline behavior playwright-03: Test Step interactions playwright-04: Test Step buttons and menu item playwright-05: Test DOM node previews on user action step hover react_devtools-02: RDT integrations (Chromium) react_devtools-03: process and display multiple React versions in page react_devtools-04: Component selection is maintained when seeking to a new point redux_devtools: Test Redux DevTools. resizable-panels-01: Left side Toolbar and Video should be collapsible restart debugging session scopes_rerender: Test that scopes are rerendered source-line-highlights: Test source line highlighting sourcemap_stacktrace: Test that stacktraces are sourcemapped stepping-01: Test basic step-over/back functionality stepping-06: Test stepping in async frames and async call stacks

Domiii · 2024-02-11T10:15:23Z

third_party/blink/renderer/bindings/core/v8/record_replay_interface.cc

@@ -825,7 +827,7 @@ const char* gOnNewWindowScript = R""""(
  window.__RECORD_REPLAY__ = window.top.__RECORD_REPLAY__;
  window.__RECORD_REPLAY_ARGUMENTS__ = window.top.__RECORD_REPLAY_ARGUMENTS__;

-  for (g of window.top.__RECORD_REPLAY_GLOBALS__) {
+  for (const g of window.top.__RECORD_REPLAY_GLOBALS__) {


Note: My previous concern was not addressed -

We also need to pass down __RECORD_REPLAY_GLOBALS__ itself.

In fact, I'd vote to only pass down __RECORD_REPLAY_GLOBALS__, and just force any user of this API to go through that, rather than adding random globals into a global context that is shared with arbitrary user JS.

Allow backend to send commands to execute preamble scripts.

7d41093

klochek requested review from Domiii and cweld510 February 7, 2024 21:01

Domiii reviewed Feb 8, 2024

View reviewed changes

Fixes.

8805488

cweld510 reviewed Feb 9, 2024

View reviewed changes

klochek force-pushed the cklochek/preamble branch from cf0ac5c to 8805488 Compare February 9, 2024 15:49

Domiii reviewed Feb 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow backend to send commands to execute preamble scripts. #1122

Allow backend to send commands to execute preamble scripts. #1122

klochek commented Feb 7, 2024

Domiii Feb 8, 2024 •

edited

Loading

Domiii commented Feb 8, 2024

Domiii Feb 8, 2024 •

edited

Loading

cweld510 Feb 9, 2024

klochek Feb 9, 2024

cweld510 commented Feb 8, 2024

cweld510 left a comment

cweld510 Feb 9, 2024

klochek Feb 9, 2024

cweld510 Feb 9, 2024

klochek Feb 9, 2024

cweld510 Feb 9, 2024

cweld510 Feb 9, 2024

klochek commented Feb 9, 2024

replay-io bot commented Feb 9, 2024 •

edited

Loading

Domiii Feb 11, 2024

Allow backend to send commands to execute preamble scripts. #1122

Are you sure you want to change the base?

Allow backend to send commands to execute preamble scripts. #1122

Conversation

klochek commented Feb 7, 2024

Domiii Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

Domiii commented Feb 8, 2024

Domiii Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cweld510 commented Feb 8, 2024

cweld510 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klochek commented Feb 9, 2024

replay-io bot commented Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

Domiii Feb 8, 2024 •

edited

Loading

Domiii Feb 8, 2024 •

edited

Loading

replay-io bot commented Feb 9, 2024 •

edited

Loading